feat(core): set Substrait version and producer #406
Conversation
nielspardon
force-pushed
the
par-spec-version
branch
5 times, most recently
from
c5118c0 to
2264345
Compare
nielspardon
force-pushed
the
par-spec-version
branch
from
2264345 to
6929ab3
Compare
vbarua
left a comment
vbarua
left a comment
There was a problem hiding this comment.
Thanks for working on this ✨
Overall this looks great, though I did have some relatively minor comments.
|
|
||
| PlanProtoConverter planToProto = new PlanProtoConverter(); | ||
|
|
||
| return planToProto.toProto(builder.build()); |
There was a problem hiding this comment.
Good call on wiring this through the POJOs
| } | ||
|
|
||
| @Value.Immutable | ||
| public abstract static class Version { |
There was a problem hiding this comment.
One small thing I would suggest here is splitting the general concept of the Version POJO from the substrait-java specific Version that we want to create. That is, other java based systems might use the POJOs to manipulate Substrait plans and might want to set their own versions, which gets a bit muddied if we include defaults in the definition IMO.
What do you think about something like:
@Value.Immutable
public abstract static class Version {
abstract int getMajor();
abstract int getMinor();
abstract int getPatch();
abstract Optional<String> getGitHash();
abstract Optional<String> getProducer();
public static ImmutableVersion.Builder builder() {
return ImmutableVersion.builder();
}
}
public static final Version SUBSTRAIT_JAVA;
static {
final String[] VERSION_COMPONENTS = loadVersion();
SUBSTRAIT_JAVA =
Version.builder()
.minor(Integer.parseInt(VERSION_COMPONENTS[1]))
.patch(Integer.parseInt(VERSION_COMPONENTS[2]))
.producer("substrait-java")
.build();
}which keeps the POJO object generic in that it models any Version, but we provide a pre-populated Version for folks that want to use it?
There was a problem hiding this comment.
we can change that sure. I did set a default mainly because the substrait-validator complains if it's not set at all so I thought it makes sense to set it by default in any case. the way it's implemented with @Value.Default would still allow users to override the values. instead of doing it for the individual version components we can also do it at the full version object level.
There was a problem hiding this comment.
I pushed an update to this code:
@Value.Immutable
public abstract class Plan {
@Value.Default
public Version getVersion() {
return Version.DEFAULT_VERSION;
}
..
@Value.Immutable
public abstract static class Version {
public static final Version DEFAULT_VERSION;
static {
final String[] versionComponents = loadVersion();
DEFAULT_VERSION =
ImmutableVersion.builder()
.major(Integer.parseInt(versionComponents[0]))
.minor(Integer.parseInt(versionComponents[1]))
.patch(Integer.parseInt(versionComponents[2]))
.producer(Optional.of("substrait-java"))
.build();
}
6929ab3 to
e7f52ce
Compare
Signed-off-by: Niels Pardon <par@zurich.ibm.com>
nielspardon
force-pushed
the
par-spec-version
branch
from
e7f52ce to
5031b4b
Compare
The only change is the addition of the `Version` field, which is expected given substrait-io/substrait-java#406. Signed-off-by: Ingo Müller <ingomueller@google.com>
The only change is the addition of the `Version` field, which is expected given substrait-io/substrait-java#406. Signed-off-by: Ingo Müller <ingomueller@google.com>
3 participants
This PR sets the Substrait specification version in the Plan by default which is supposed to be required since Substrait 0.17.0 according to this.
The Substrait specification version is being retrieved from the
substraitGit submodule by runninggit describe --tagsvia Gradle.The Substrait specification version is then written into a
MANIFEST.MFfile together with the substrait-java version following the Java manifest conventions:Version.class.getPackage().getSpecificationVersion()which is only available when theVersionclass is being used from a JAR containing theMANIFEST.MFMANIFEST.MFfile from the classpath as a resource if we are using theVersionclass from within the Gradle build of substrait-java since then the class is not part of a JAR.In order to make the unit tests work I simplified the
SqlToSubstrait.executeInner()method by first creating a Substrait Java SDKPlanwhich then gets converted to a proto Plan instead of creating the proto Plan directly. Otherwise the proto Plan would not have contained the default version and producer values and some of the round tripping tests failed.Finally, I'm adding one more entry to gitignore which is created by VSCode's Scala tooling.
I had to modify the Github actions configuration to make sure the submodule Git tags exist during the build.
Contributes to (maybe even fixes) #253